Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dev/core#2823 Extract code to load the declarations and call from the constructor #21399

Merged
merged 2 commits into from
Sep 11, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Sep 8, 2021

Overview

dev/core#2823 Extract code to load the declarations and call from the constructor

Before

declarations are loaded in getDeclarations if not already loaded. However, there is also code to load them in the constructor which makes is confusing

After

Always loaded in the constructor

Technical Details

The declarations are only used in object context so it makes sense to load
them in the constructor and they are always needed in usage of this class.

In addition they are ALWAYS loaded (rather than passed in) except in test usage
(it does seem a bit silly having the option to pass them in only for tests
but we can ignore that for now - I commented it)

Comments

@civibot
Copy link

civibot bot commented Sep 8, 2021

(Standard links)

@civibot civibot bot added the master label Sep 8, 2021
@eileenmcnaughton eileenmcnaughton changed the title dev/core#2823 Extract code to load the declarations and call from the… dev/core#2823 Extract code to load the declarations and call from the constructor Sep 8, 2021
As the code comments suggest the handling of a module being unrecognised sould
be handled in the validate not the enable function
… constructor

The declarations are only used in object context so it makes sense to load
them in the constructor. In addition they are ALWAYS loaded except in test usage
(it does seem a bit silly having the option to pass them in only for tests
but we can ignore that for now - I commented it)
@colemanw
Copy link
Member

This passes unit tests so it must be ok, but it's strange that #21401 fails.
Let's merge this and then see where we stand with the other.

@colemanw colemanw merged commit 741ebf1 into civicrm:master Sep 11, 2021
@colemanw colemanw deleted the mgd branch September 11, 2021 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants